-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix miscellaneous code quality and consistency issues #4766
Fix miscellaneous code quality and consistency issues #4766
Conversation
/proc/qdel(datum/D, force=FALSE) | ||
if(isnull(D)) | ||
return | ||
if(!istype(D)) | ||
PRINT_STACK_TRACE("qdel() can only handle /datum (sub)types, was passed: [log_info_line(D)]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not want qdel() on non-instances to error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
images :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this response. Images harddel'd prior to this PR so I'm not sure what the purpose of these changes is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? images still get qdel'd though. you literally added code in the footprints pr that qdel_list'd a list of images. and it should be fine to qdel an image, but this code makes that throw a useless error. istype(/image, /datum) is false, it will throw a pointless error if you try to qdel an image, and if you do this you can't do QDEL_IN(image, X)
. the only real case where it should error is if we pass a primitive like text or a number, which shouldn't get del'd anyway, and if you want i can add a CRASH
for that but it seems unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation.
The qdel list in footprints is a mistake (per Discord) - I'm not sure we should be implicitly supporting the qdel of images/lists, since they're better left to the GC. No strong feelings on the matter though. The CRASH on an invalid qdel param has been useful in the past for me however.
fc74ca3
to
c3e4150
Compare
c3e4150
to
6d73ca0
Compare
Description of changes
Removes a lot of spawn() and sleep() abuse in favor of QDEL_IN and timers.
Makes a bunch of things use abstract_type instead of bespoke vars that do the same thing.
Makes attack filters a datum subtype.
Makes the prometheus metrics system use decls.
Fixes some old outdated code comments.
Fixes horse coloration.
Why and what will this PR improve
Improves code quality and consistency.